-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
deprecate invwarpedview
in favor of InvWarpedView
#138
Conversation
This also introduces the same set of already deprecated API to keep consistant with `warpedview`, e.g., `InvWarpedView(img, tinv, inds, method, fillvalue)` is newly added as deprecated method.
Codecov Report
@@ Coverage Diff @@
## master #138 +/- ##
==========================================
- Coverage 92.15% 91.09% -1.06%
==========================================
Files 8 8
Lines 204 191 -13
==========================================
- Hits 188 174 -14
- Misses 16 17 +1
Continue to review full report at Codecov.
|
This seems reasonable. One distinction between |
The more concerning issue I find with this trick with this "license" to merge two or more lazy warp views through img_camera = testimage("cameraman")
img = similar(img_camera) # uninitialized array
v = view(img, 75:195, 245:370)
v .= img_camera[v.indices...]
tfm = recenter(RotMatrix(-pi/8), center(img_camera))
wrong_result = collect(invwarpedview(v, tfm)) # there's no way to check if `parent(v)` are filled with real useful data.
correct_result = collect(InvWarpedView(v, tfm))
mosaic(wrong_result, correct_result; nrow=1) wrong_result = collect(invwarpedview(v, tfm, axes(v)))
correct_result = collect(InvWarpedView(v, tfm, axes(v)))
mosaic(wrong_result, correct_result; nrow=1) It should be the caller's responsibility to check whether this optimization is valid and not the callee |
I like this way of thinking! 100% agreed. |
Should be the last PR before v0.9 release.
Now that
InvWarpedView
acceptsmethod
andfillvalue
keywords, and also commits to theparent(wv) === img
assumption. (This is also an improvement to #24 and thus makesinvwarpedview
not really necessary.)This also introduces the same set of already deprecated API to keep consistent with
warpedview
, e.g.,InvWarpedView(img, tinv, inds, method, fillvalue)
is newly added as a deprecated method.Other unrelated changes:
@inbounds
forgetindex
method for bothWarpedView
andInvWarpedView
Base.size
methods and removeIndexStyle
; it's sufficient to use the fallback implementation in Base.TODO:
SubArray
trick back appropriately (This trick is directly removed)